Skip to content

[SPARK-20569][SQL] RuntimeReplaceable functions should not take extra parameters#17876

Closed
cloud-fan wants to merge 1 commit intoapache:masterfrom
cloud-fan:minor
Closed

[SPARK-20569][SQL] RuntimeReplaceable functions should not take extra parameters#17876
cloud-fan wants to merge 1 commit intoapache:masterfrom
cloud-fan:minor

Conversation

@cloud-fan
Copy link
Copy Markdown
Contributor

@cloud-fan cloud-fan commented May 5, 2017

What changes were proposed in this pull request?

RuntimeReplaceable always has a constructor with the expression to replace with, and this constructor should not be the function builder.

How was this patch tested?

new regression test

@cloud-fan
Copy link
Copy Markdown
Contributor Author

cloud-fan commented May 5, 2017

cc @gatorsmile @marmbrus

@SparkQA
Copy link
Copy Markdown

SparkQA commented May 5, 2017

Test build #76501 has finished for PR 17876 at commit 431a8cd.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link
Copy Markdown

SparkQA commented May 8, 2017

Test build #76570 has finished for PR 17876 at commit fcbeeb9.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link
Copy Markdown

SparkQA commented May 8, 2017

Test build #76587 has finished for PR 17876 at commit 0021ec3.

  • This patch fails SparkR unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@cloud-fan
Copy link
Copy Markdown
Contributor Author

cloud-fan commented May 9, 2017

retest this please

@SparkQA
Copy link
Copy Markdown

SparkQA commented May 9, 2017

Test build #76599 has finished for PR 17876 at commit 0021ec3.

  • This patch fails SparkR unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@cloud-fan
Copy link
Copy Markdown
Contributor Author

cloud-fan commented May 9, 2017

retest this please

@SparkQA
Copy link
Copy Markdown

SparkQA commented May 9, 2017

Test build #76609 has finished for PR 17876 at commit 0021ec3.

  • This patch fails SparkR unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link
Copy Markdown

SparkQA commented May 9, 2017

Test build #76616 has finished for PR 17876 at commit 601e988.

  • This patch fails SparkR unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gatorsmile
Copy link
Copy Markdown
Member

gatorsmile commented May 9, 2017

retest this please

@SparkQA
Copy link
Copy Markdown

SparkQA commented May 9, 2017

Test build #76654 has finished for PR 17876 at commit 601e988.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

gatorsmile
gatorsmile reviewed May 11, 2017
View reviewed changes
Comment thread sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
val maxNumArgs = all.map(_.getParameterCount).max
all.filterNot(_.getParameterCount == maxNumArgs)
} else {
tag.runtimeClass.getConstructors
Copy link
Copy Markdown
Member

@gatorsmile gatorsmile May 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason why we originally called getDeclaredConstructor instead of getConstructor?

Copy link
Copy Markdown
Contributor Author

@cloud-fan cloud-fan May 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getDeclaredConstructor will find a specific constructor matching the given parameter types. Now I have some special logic about choosing the constructor, so I call getConstructors to get all the constructors.

@gatorsmile
Copy link
Copy Markdown
Member

gatorsmile commented May 11, 2017

LGTM

asfgit pushed a commit that referenced this pull request May 11, 2017
@cloud-fan @gatorsmile
[SPARK-20569][SQL] RuntimeReplaceable functions should not take extra…
dd9e3b2 
… parameters

## What changes were proposed in this pull request?

`RuntimeReplaceable` always has a constructor with the expression to replace with, and this constructor should not be the function builder.

## How was this patch tested?

new regression test

Author: Wenchen Fan <wenchen@databricks.com>

Closes #17876 from cloud-fan/minor.

(cherry picked from commit b4c99f4)
Signed-off-by: Xiao Li <gatorsmile@gmail.com>
@gatorsmile
Copy link
Copy Markdown
Member

gatorsmile commented May 11, 2017 •
edited
Loading

Thanks! Merging to master/2.2

@asfgit asfgit closed this in b4c99f4 May 11, 2017
lycplus pushed a commit to lycplus/spark that referenced this pull request May 24, 2017
@cloud-fan @lycplus
[SPARK-20569][SQL] RuntimeReplaceable functions should not take extra…
501fc78 
… parameters

## What changes were proposed in this pull request?

`RuntimeReplaceable` always has a constructor with the expression to replace with, and this constructor should not be the function builder.

## How was this patch tested?

new regression test

Author: Wenchen Fan <wenchen@databricks.com>

Closes apache#17876 from cloud-fan/minor.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gatorsmile gatorsmile gatorsmile left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cloud-fan @SparkQA @gatorsmile